Skip to content

Descriptor V2#499

Merged
davidbrai merged 122 commits intomasterfrom
verbs-descriptor-v2
Jul 19, 2022
Merged

Descriptor V2#499
davidbrai merged 122 commits intomasterfrom
verbs-descriptor-v2

Conversation

@davidbrai
Copy link
Collaborator

@davidbrai davidbrai commented Jun 30, 2022

A new descriptor version that allows for significantly cheaper deployment of the Nouns art and better composability by improving the external API.

The new contracts provides the following benefits:

Cheaper deployment cost: ~13.8M gas instead of ~67.3M gas
Easier composability for external contracts by splitting the tokenURI construction to several contracts

Full tech spec: https://github.com/nounsDAO/nouns-tech/blob/main/descriptor-v2/README.md

This also introduces some fixes to the art.

"nounsDAOLogicV1": "0xa43aFE317985726E4e194eb061Af77fbCb43F944"
},
"4": {
"nounsToken": "0x632f34c3aee991b10D4b421Bc05413a03d7a37eB",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@solimander is it important to keep the original values here before merging to master?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's okay to keep the new Rinkeby contract addresses as long as they're targeted in the subgraph that's used in the webapp.

{
"network": "rinkeby",
"nounsToken": {
"address": "0xc52bb4Fc4ed72f2a910BF0481D620B927Ded76f7",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@solimander is it important to revert these values before merging to master?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to #499 (comment), we can keep these new values as long as the new values also remain in the SDK.

@davidbrai davidbrai changed the title [DO NOT MERGE] Draft: descriptor v2 Descriptor V2 Jul 18, 2022
@davidbrai davidbrai marked this pull request as ready for review July 18, 2022 16:41
Comment on lines +21 to +24
.addOptionalParam('auctionDuration', 'The auction duration (seconds)', 600, types.int)
.addOptionalParam('timelockDelay', 'The timelock delay (seconds)')
.addOptionalParam('votingPeriod', 'The voting period (blocks)')
.addOptionalParam('votingDelay', 'The voting delay (blocks)')
.addOptionalParam('votingPeriod', 'The voting period (blocks)', 5760, types.int)
.addOptionalParam('votingDelay', 'The voting delay (blocks)', 1, types.int)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't add defaults here so the default values in the deploy task would be used. Is there an issue with those? Vaguely remember some type issue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eladmallel do you remember why these were added?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iainnash
Copy link

Is it worthwhile to think about including a naming extension as well with this for properties?

#479
Is a wrapper contract that achieves this but with an upgrade names should be fairly trivial to add.

@solimander
Copy link
Collaborator

solimander commented Jul 18, 2022

Is it worthwhile to think about including a naming extension as well with this for properties?

@iainnash Appreciate the contribution, but it's the wish of the Noun artists that the trait names do not appear in the metadata.

not specifying default values falls back on the default values
in the deploy task
Copy link
Collaborator

@solimander solimander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@davidbrai davidbrai merged commit 5a49fd4 into master Jul 19, 2022
@volkyeth volkyeth deleted the verbs-descriptor-v2 branch May 5, 2025 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants